Fix various edge case bugs in QValidatedLineEdit #404
Conversation
There was a problem hiding this comment.
I looked through the PR and the changes that are made. I have got some doubts that I want to ask the op regarding some changes made here:
- The default color property is now enclosed in a CSS selector to avoid changing the tooltip's background color. But was that a problem in the first place?
If enclosing the color property has its benefits, shouldn’t a similar thing be done in line 33setStyleSheet("");to maintain consistency?- The validity of text is checked after setting checkValidator in the setCheckValidator function. Why is there a need to check the validity here?
It would be helpful if op could clarify these doubts. Thank You.
Yes. Without it, the tooltip bg colour is changed too. That is incorrect behaviour.
No, that doesn't make sense... Do you know CSS? :/
It's the logically correct thing to do to ensure a consistent state. |
That makes sense. And I would agree with it.
As a matter of fact, I do know CSS. I wrongly interpreted the changes introduced here, and that was a genuine mistake on my part. Let me go through some further testings to check if I am not missing something. In the meantime, @luke-jr, it would be great if you could post some screenshots that clearly express the differences introduced in this PR. |
There was a problem hiding this comment.
Concept ACK fixing and improving this custom widget. Does anyone know why https://doc.qt.io/qt-5/qlineedit.html#setValidator is not used?
There was a problem hiding this comment.
@luke-jr, just following up on this comment: bitcoin/bitcoin#15987 (comment)
Why would bitcoin/bitcoin#15987 depend on this?
|
Because without this, validators don't work correctly. bitcoin/bitcoin#15987 uses a validator. Additionally, inline marking of bech32 error detection (#527) also depends on this. |
…ineEdit aeb18b6 Bugfix: GUI: Check validity when QValidatedLineEdit::setText is called (Luke Dashjr) b1a544b Bugfix: GUI: Re-check validity after QValidatedLineEdit::setCheckValidator (Luke Dashjr) 2385b50 Bugfix: GUI: Only apply invalid style to QValidatedLineEdit, not its tooltip (Luke Dashjr) Pull request description: 1. Use a CSS selector to avoid changing the background colour of the tooltip. 2. Re-check validity of input when we first set the validator (probably a no-op in practice). 3. Check validity of input when it is set programmatically via `setText` (eg, via the address book). Probably no-op in practice UNTIL merging bitcoin#15987 or any other PR that adds a warning for valid addresses. Moved from bitcoin#18133 (just concept ACKs) ACKs for top commit: Sjors: tACK aeb18b6 hebasto: ACK aeb18b6, tested on Linux Mint 20.3 (Qt 5.12.8). Tree-SHA512: b6fa8ee4dec76e1c759095721240e6fa5071a02993cb28406e96a0fa2e819f5dddc03d2e7c9073354d7865c2b09eb263afaf853ecba42e9fc4f50ef4ae20bf0f
setText(eg, via the address book). Probably no-op in practice UNTIL merging Wallet, GUI: Warn when sending to already-used Bitcoin addresses bitcoin/bitcoin#15987 or any other PR that adds a warning for valid addresses.Moved from bitcoin/bitcoin#18133 (just concept ACKs)